Signer: Fix loading of per-signature private keys with different types #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This keeps the key type determination logic in one place.
If the key file comes from the Signature object, use the algorithm of that Signature object to determine the key type.
The Signature constructor does not make use of the KeyFile property.
dev-aaront-org
force-pushed
the
signer-key-type
branch
from
7f6e506 to
1441dd3
Compare
|
I was thinking about this some more and I realized that my fix enabled some nonsense scenarios where the key file could come from |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a
Signatureobject'sKeyorKeyFileproperty is set to a non-ref,Signerassumes it is a file path and attempts to load the private key from it, but it may use the wrong key type, which will cause the signature to fail. CurrentlySignerlooks at$self->{Algorithm}to determine the key type, and this changes it to look at theSignature's algorithm instead.I'm not entirely sure this fix is right or necessary because it doesn't really look like a supported use case. The docs for
Signaturesay thatKeyshould be an object, andKeyFileis not used at all withinSignature. So I was also considering just removing the key loading fromSignerand making it the user's responsibility to load their own keys if they're creating their ownSignatures. But that would be a backward-incompatible change.Anyway, I'm certainly open to suggestions if you prefer a different fix.